Cache DocumentDiagnosticAnalyzer results by version stamp to avoid redundant typecheck on unchanged documents - #20121
Conversation
xperiandri
changed the title
DocumentDiagnosticAnalyzer results by version stamp to avoid redundant typecheck on unchanged documents
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
| document.Project.Solution.GetFSharpExtensionConfig().ShouldProduceDiagnostics() | ||
|
|
||
| static let cache = | ||
| ConcurrentDictionary<struct (DocumentId * DiagnosticsType), VersionStamp * VersionStamp * ImmutableArray<Diagnostic>>() |
There was a problem hiding this comment.
Unbounded static cache / memory retention. This static ConcurrentDictionary is keyed by struct (DocumentId * DiagnosticsType) and has no eviction path. Edits to the same document overwrite their own key, so growth is bounded by the number of distinct documents ever opened (×2 for Syntax/Semantic) — but entries for documents that are closed or removed from the solution are never freed and persist for the entire lifetime of the devenv.exe process. Each retained entry holds an ImmutableArray<Diagnostic> (with Location/file-path data), so in a long-running session over a large solution this is steady, unbounded retention.
Consider evicting on workspace changes — e.g. subscribe to Workspace.WorkspaceChanged and drop entries on DocumentRemoved/DocumentRemovedFromSolution (and optionally when a document is closed), or cap the cache size. The referenced emitCache in FSharpProjectOptionsManager.fs is keyed by project reference and thus has far smaller cardinality than per-document diagnostics, so the retention surface here is larger.
4592d35 to
fbec9a8
Compare
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev Caution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository: `* . (PR #XXXXX)`
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.
|
|
When Transparent Compiler is in use, typecheck results, along with their diagnostics are already cached on the FCS side. One thing that comes to mind: it wouldn't be hard to cache also unused opens and unused declarations results using Transparent Compiler caching. |
…recomputing diagnostics when document/project version is unchanged
fbec9a8 to
47373d0
Compare
3 participants
Fixes #20120
Summary
FSharpDocumentDiagnosticAnalyzer.GetDiagnosticspreviously recomputed syntax/semantic diagnostics (parse, typecheck,UnusedParentheses) on every crawler pass, even when the document text and project state had not changed since the last computation.This adds a version-stamp-aware cache keyed by
struct (DocumentId * DiagnosticsType), storing the last computed(textVersion, projectVersion, ImmutableArray<Diagnostic>)tuple:Syntaxdiagnostics, only the documenttextVersionis tracked (projectVersionusesVersionStamp.Default).Semanticdiagnostics, bothtextVersionanddocument.Project.GetDependentVersionAsync()are tracked.UnnecessaryParenthesesDiagnosticAnalyzerwork entirely.This mirrors the versioned-cache pattern used for referenced-project compilation emission in
FSharpProjectOptionsManager.fs(emitCache).Testing
dotnet build vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj -c Debug— succeeded.Build.cmd -c Debug -deployExtensions) and validated with a CPU trace ofdevenv.exe.